fix(components/docs-tools): links in if statements are correctly registered in table of contents#4339
fix(components/docs-tools): links in if statements are correctly registered in table of contents#4339Blackbaud-ErikaMcVey wants to merge 1 commit intomainfrom
Conversation
…stered in table of contents
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
View your CI Pipeline Execution ↗ for commit 51fc32c
☁️ Nx Cloud last updated this comment at |
|
Component Storybooks:
Apps: |
| // rather than the order at which they were registered. | ||
| for (const el of els) { | ||
| this.#anchors.forEach((anchor) => { | ||
| if (anchor.equals(el)) { |
There was a problem hiding this comment.
This public method is no longer used on the anchor component; can you remove it?
There was a problem hiding this comment.
Pull request overview
This PR fixes an issue in @skyux/docs-tools where heading anchors rendered inside structural directives (e.g., @if) could fail to appear in the table of contents due to DOM-query-based sorting during registration.
Changes:
- Update
SkyDocsHeadingAnchorServiceto track each registered anchor’s native element in aMapand sort anchors viacompareDocumentPositioninstead ofdocument.querySelectorAll(...). - Fix
unregister()to remove only the target anchor (splice(index, 1)). - Update
SkyDocsHeadingAnchorComponentto pass its native element into the service during registration.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| libs/components/docs-tools/src/lib/modules/heading-anchor/heading-anchor.service.ts | Stores element refs for registered anchors, sorts links by element document position, and fixes unregister removal. |
| libs/components/docs-tools/src/lib/modules/heading-anchor/heading-anchor.component.ts | Passes the host element to the service during registration. |
| if (!this.#anchors.includes(anchor)) { | ||
| this.#anchors.push(anchor); | ||
| this.#anchorElements.set(anchor, element); | ||
| this.#anchorsChange.next(this.#getLinks()); | ||
| } |
There was a problem hiding this comment.
register() emits anchorsChange synchronously. In the reported @if timing scenario the host element may still be disconnected from document, so #getLinks() may compute an implementation-specific order (and never re-emit once the element is attached). Consider deferring the emission (e.g., queueing a microtask / next tick) or otherwise scheduling a second recomputation after the element is connected so the TOC order reliably reflects actual DOM order.
| const position = elA.compareDocumentPosition(elB); | ||
|
|
||
| if (position & Node.DOCUMENT_POSITION_FOLLOWING) { | ||
| return -1; | ||
| } | ||
|
|
||
| if (position & Node.DOCUMENT_POSITION_PRECEDING) { | ||
| return 1; | ||
| } | ||
|
|
||
| return 0; |
There was a problem hiding this comment.
The sort comparator uses compareDocumentPosition(), but it doesn't account for Node.DOCUMENT_POSITION_DISCONNECTED. For disconnected nodes browsers may also set PRECEDING/FOLLOWING in an implementation-specific way, which can produce a stable but incorrect DOM order. Consider explicitly checking for DISCONNECTED and returning 0 (or another deterministic fallback) until both nodes share a document/root.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
libs/components/docs-tools/src/lib/modules/heading-anchor/heading-anchor.service.ts (1)
25-34: Consider adding a guard for SSR compatibility.The
compareDocumentPositionDOM API is only available in browser contexts. While this docs-tools module likely only runs in the browser, if there's any chance of SSR usage, a platform check could prevent runtime errors.🛡️ Optional: Add platform check if SSR is ever a concern
+import { Injectable, OnDestroy, PLATFORM_ID, inject } from '@angular/core'; +import { isPlatformBrowser } from '@angular/common'; `@Injectable`() export class SkyDocsHeadingAnchorService implements OnDestroy { + readonly `#platformId` = inject(PLATFORM_ID); `#anchors`: SkyDocsHeadingAnchorComponent[] = []; `#anchorElements` = new Map<SkyDocsHeadingAnchorComponent, Element>();Then in
register():public register(anchor: SkyDocsHeadingAnchorComponent, element: Element): void { if (!isPlatformBrowser(this.#platformId)) { return; } // ... existing logic }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/components/docs-tools/src/lib/modules/heading-anchor/heading-anchor.service.ts` around lines 25 - 34, The register method should guard against server-side execution to avoid using DOM-only APIs; add a platform check at the start of register (e.g., use isPlatformBrowser(this.#platformId)) and return early when not in a browser before touching `#anchors`, `#anchorElements`, or calling `#anchorsChange.next`; ensure the service has access to the injected platform id (this.#platformId) and import isPlatformBrowser so the method short-circuits in SSR environments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@libs/components/docs-tools/src/lib/modules/heading-anchor/heading-anchor.service.ts`:
- Around line 25-34: The register method should guard against server-side
execution to avoid using DOM-only APIs; add a platform check at the start of
register (e.g., use isPlatformBrowser(this.#platformId)) and return early when
not in a browser before touching `#anchors`, `#anchorElements`, or calling
`#anchorsChange.next`; ensure the service has access to the injected platform id
(this.#platformId) and import isPlatformBrowser so the method short-circuits in
SSR environments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ee3b8297-55b6-4a85-85e1-8e2533a46184
📒 Files selected for processing (2)
libs/components/docs-tools/src/lib/modules/heading-anchor/heading-anchor.component.tslibs/components/docs-tools/src/lib/modules/heading-anchor/heading-anchor.service.ts
Claude's explanation of the issue:
Problem:
SkyDocsHeadingAnchorService.#getLinks()useddocument.querySelectorAll('sky-docs-heading-anchor')to find and sort registered anchors by DOM position. When heading anchors were rendered inside structural directives like@if, Angular'sngAfterViewInitfired (triggering register) before the elements were attached to the document. SincequerySelectorAllcouldn't find those elements, they were silently dropped from the emitted links array, causing the table of contents to appear empty.Fix: Instead of querying the DOM, the service now stores each anchor's native element reference in a
Mapduringregister(). The#getLinks()method sorts usingcompareDocumentPositionon those stored references, ensuring all registered anchors are always included regardless of their attachment timing. A secondarysplicebug (missing thedeleteCountargument, causing it to remove all anchors after the target) was also fixed.Summary by CodeRabbit